fix(code-review): Consolidate code review checks#105561
fix(code-review): Consolidate code review checks#105561suejung-sentry merged 8 commits intomasterfrom
Conversation
armenzg
left a comment
There was a problem hiding this comment.
Well done! 🎉
Besides tests, could you write a ticket to help other parts of Sentry use your logic? (or see how easy it is to switch over) That way we have a centralized place and we prevent unintentional regressions.
Until we officially freeze the code review beta list, consider anyone with the feature toggle ON as part of the beta too. Note that we'll replace this with the logic [here](#105561) shortly, but wanted to unblock this.
d9e3314 to
9a8da6d
Compare
Here's a ticket CW-111. I think easiest is to do a hard cutover to this new flow path and the old one within overwatch can just die on the vine, but we can see how the timing works out. |
| except OrganizationContributors.DoesNotExist: | ||
| metrics.incr( | ||
| "overwatch.code_review.contributor_not_found", | ||
| tags={"organization_id": organization.id, "repository_id": repository_id}, |
There was a problem hiding this comment.
Note that when I moved this over I removed the tag for repository id as I thought it would add too much cardinality on the metric. Will connect with @ajay-sentry if that is going to be a problem..
| organization: The Sentry organization that the webhook event belongs to | ||
| repo: The repository that the webhook event is for | ||
| **kwargs: Additional keyword arguments including integration | ||
| integration: The GitHub integration |
There was a problem hiding this comment.
Looks like we need integration for the preflight billing check
| pr_author_external_id=get_pr_author_id(event), | ||
| ).check() | ||
| if not preflight.allowed: | ||
| # TODO: add metric |
There was a problem hiding this comment.
Will add a metric consistent with any new standards in this PR - https://github.com/getsentry/sentry/pull/105587/files
|
|
||
| @patch("sentry.seer.code_review.webhooks.task.process_github_webhook_event") | ||
| @with_feature({"organizations:gen-ai-features"}) | ||
| def test_check_run_runs_when_code_review_beta_flag_disabled_but_pr_review_test_generation_enabled( |
There was a problem hiding this comment.
this testcase is covered in the preflight class tests
| } | ||
|
|
||
|
|
||
| def get_pr_author_id(event: Mapping[str, Any]) -> str | None: |
There was a problem hiding this comment.
armenzg
left a comment
There was a problem hiding this comment.
Good from my POV. Feel free to merge.
You may want input from someone familiar with billing for the tests you added.
| integration_id=integration.id if integration else None, | ||
| pr_author_external_id=get_pr_author_id(event), | ||
| ).check() | ||
| if not preflight.allowed: |
There was a problem hiding this comment.
Great to see it being handled in the main handler function 🦾 .
44295f1 to
d46d0e1
Compare
Thanks, I'll connect with @ajay-sentry when he's back on Wednesday. The billing checks aren't live until we GA so should be OK to merge in the meantime |
| def _check_billing(self) -> DenialReason: | ||
| return None | ||
|
|
||
| # TODO: Once we're ready to actually gate billing (when it's time for GA), uncomment this |
There was a problem hiding this comment.
Note that I updated this to always let the billing check go through for now since it's not turned on for the overwatch flow yet either (just logged here). This will be turned on with GA.
de5360b to
67c1c83
Compare
| if (user_id := event.get("user", {}).get("id")) is not None: | ||
| return str(user_id) | ||
|
|
||
| # Check sender.id (for check_run events). Sender is the user who triggered the event |
There was a problem hiding this comment.
I'll confirm with @ajay-sentry / Product who/what we want to increment for this webhook event. It's possible they would want this one to be "always free"
67c1c83 to
ddea878
Compare
ddea878 to
3de8ad8
Compare
armenzg
left a comment
There was a problem hiding this comment.
Some of this code I did not merge into my PR since it did not seem necessary. Please let me know if I got it wrong!
| github_integration: Integration | None = None | ||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mock_billing_quota(self) -> Generator[None]: |
There was a problem hiding this comment.
When I merged this code into #105673 I did not port this fixture.
| self.organization.update_option("sentry:enable_pr_review_test_generation", True) | ||
|
|
||
| # Setup billing data | ||
| self.github_integration = self.create_github_integration() |
| self.organization.update_option("sentry:enable_pr_review_test_generation", True) | ||
|
|
||
| # Setup billing data | ||
| self.github_integration = self.create_github_integration() |
| repo_id = int(self.event_dict["repository"]["id"]) | ||
|
|
||
| integration = self.create_github_integration() | ||
| integration = self.github_integration or self.create_github_integration() |
| self.mock_seer.assert_not_called() | ||
|
|
||
| @with_feature({"organizations:gen-ai-features"}) | ||
| def test_runs_when_code_review_beta_flag_disabled_but_pr_review_test_generation_enabled( |
There was a problem hiding this comment.
Merged as this:
sentry/tests/sentry/seer/code_review/webhooks/test_issue_comment.py
Lines 100 to 110 in 463c021
Consolidate all the different checks for code review to one spot so we can use it in our new webhook flow.
Note that I left the billing check off for now and will check with the team on how best to transition it on for GA
Closes CW-88